-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Network-interfaces.target #18491
Conversation
@groxxda, thanks for your PR! By analyzing the annotation information on this pull request, we identified @wkennington, @offlinehacker and @rick68 to be potential reviewers |
I just rebased against current master with the changes from #18319 |
Hm, network.target is supposed to mean that networking functionality is "available". To me, the intuitive choice is to order dhcpcd after network.target. Ordering before/after causes dhcpcd to be stopped after/before network.target on shutdown, not sure if that makes any practical difference to us, however. |
|
The changes to scripted network interfaces look fine to me. |
@groxxda how are you feeling about this? Is it ready from your pov? I think it's been up long enough that we can begin thinking about integrating it. |
@joachifm I would be happy to see this merged. The closer we are to the next release the higher the risk will be. |
I'm working on integrating this today, going to do some testing and stuff just to be sure we're not breaking the world :) |
Thanks @joachifm 🎉 Let me know if there's something I can do. I'm happy to help out to get these changes merged |
@groxxda could you run a few of the nixos tests that might be affected? In particular, stuff like misc, etcd, openssh, firewall, and any other that looks important to you. |
I'm having a problem actually getting tests to run locally atm, stuff is just timing out on me for no obvious reason (with and without this PR). |
@joachifm I'll see if I can run some tests. Last time qemu segfaulted sporadically giving false-negatives.. By the way you should be able to push directly to my branch ( |
I think we'll have to accept that some regressions may be introduced by this, so it's not a complete blocker seeing as nobody has brought up any fundamental objections thus far, but it'd be nice to not break everything at once :) |
Tests: |
|
Alright, let's do it. |
Thanks again for reviewing and pushing this forward! 🍻 Also thanks @fpletz |
@@ -36,11 +36,16 @@ in | |||
]; | |||
|
|||
systemd.services.monit = { | |||
description = "Monit system watcher"; | |||
after = [ "network-interfaces.target" ]; | |||
description = "Pro-active monitoring utility for unix systems"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is a good description, what monit does it obscures the name if on the boot screen "Pro-active monitoring utility for unix systems started" appears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. I simply took it from the upstream file at https://bitbucket.org/tildeslash/monit/src/e02247e0480b19be88e6a908886da83bbb1bc6a3/system/startup/monit.service.in?at=master
Should we revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand the problem? Is it that it doesn't mention the name of the unit being started?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
A potential regression: https://hydra.nixos.org/build/41566104 |
@joachifm More likely one of these times out because hydra load is high failures.. |
This PR changed several units to be pulled in by For example, if I add the following to my configuration:
than this will have no effect until I reboot my system or explicitly start Probably these units should be made wanted by one of the networking targets. |
I'm also worried about the use of |
Replacing |
Yeah that sounds good. |
This reverts a change applied in PR NixOS#18491. When interfaces are configured by DHCP (typical in a cloud environment), ordering after network.target cause trouble to applications expecting some network to be present on boot (for example, cloud-init is quite brittle when network hasn't been configured for `cloud-init.service`) and on shutdown (for example, collectd needs to flush metrics on shutdown). When ordering after network.target, we ensure applications relying on network.target won't have any network reachability on boot and potentially on shutdown. Therefore, I think ordering before network.target is better.
Motivation for this change
This is ready for review and test
Systemd upstream provides 3 targets that should be used in services to allow ordering and dependencies for networking (network, network-pre, network-online)
Nixos adds at least network-interfaces.target to the list where it is not clear what it's purpose is.
To make network setup easier to understand for module developers this is an effor to get rid of the superfluous target.
This is a follow up on the removal of up-up.target and is not yet ready for merge.
I post this merely for early review and discussion (Hijacking an unrelated or was not a good idea in the first place)Mostcommits should be safe to merge(getting closer to upstream stuff)Especially the changes to the scripted network interface setup should be reviewed with care even though all networking tests passed with them.
See also https://github.com/joachifm/nixpkgs/tree/network-interfacesPing @joachifm
Next steps:
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)